Skip to content

fix(ui-buttons): make Button have a focus ring in Safari#2012

Merged
ToMESSKa merged 1 commit into
masterfrom
INSTUI-4527-focus-is-not-returned-in-safari-modal
Jun 6, 2025
Merged

fix(ui-buttons): make Button have a focus ring in Safari#2012
ToMESSKa merged 1 commit into
masterfrom
INSTUI-4527-focus-is-not-returned-in-safari-modal

Conversation

@ToMESSKa

@ToMESSKa ToMESSKa commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

INSTUI-4527

ISSUE:

  • In Safari, the same BaseButton or Button component doesn't show a focus ring, whereas the ring does appear in Chrome and other browsers - this is due to a missing 0 tabindex, which Safari needs
  • Modal trigger button does not have a focus ring after closing the Modal due to this

TEST PLAN:

  • make sure you have the latest Safari version
  • open BaseButton and Button pages in Safari
  • the first few examples should get a focus ring when they are clicked on - unlike in the latest InstUI release in Safari
  • they should have a tabIndex=0 in Safari but not in other browsers
  • open the first example of Modal in Safari, after closing it, the Modal should have a focus ring - unlike in the latest InstUI release in Safari

@ToMESSKa ToMESSKa self-assigned this Jun 4, 2025
@ToMESSKa ToMESSKa requested a review from Copilot June 4, 2025 14:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses the focus ring issue for BaseButton in Safari by adding a condition for Safari detection and updating the tabindex logic.

  • Adds a Safari-specific condition using utils.isSafari()
  • Updates tabindex assignment to ensure focusability in Safari
Comments suppressed due to low confidence (1)

packages/ui-buttons/src/BaseButton/index.tsx:283

  • [nitpick] The condition for applying a default tabindex in Safari may override an explicitly provided tabindex. Consider checking if tabindex is undefined before defaulting to 0 to ensure custom tabindex values are respected.
if ((onClick && as && needsZeroTabIndex) || (utils.isSafari() && as)) {

@github-actions

github-actions Bot commented Jun 4, 2025

Copy link
Copy Markdown
PR Preview Action v1.6.1
Preview removed because the pull request was closed.
2025-06-06 08:01 UTC

@ToMESSKa ToMESSKa requested review from balzss and joyenjoyer June 4, 2025 14:41
@ToMESSKa ToMESSKa changed the title fix(ui-buttons): make BaseButton have a focus ring in Safari fix(ui-buttons): make Button have a focus ring in Safari Jun 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please dont use * imports, they might break in Vite and also increase bundle size (because it always imports everything)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, i fixed this

@ToMESSKa ToMESSKa force-pushed the INSTUI-4527-focus-is-not-returned-in-safari-modal branch from 68c5358 to 3452796 Compare June 5, 2025 08:55
@ToMESSKa ToMESSKa requested a review from matyasf June 5, 2025 09:05
@ToMESSKa ToMESSKa merged commit 54118ac into master Jun 6, 2025
12 checks passed
@ToMESSKa ToMESSKa deleted the INSTUI-4527-focus-is-not-returned-in-safari-modal branch June 6, 2025 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants